Skip to content

Conversation

@jasonleenaylor
Copy link
Contributor

@jasonleenaylor jasonleenaylor commented Sep 19, 2025

This change is Reviewable

jasonleenaylor and others added 6 commits September 19, 2025 08:58
* Reuse wordforms that are matches but create new anaysis for them
* Add Normalization before string comparison
* Split Morpheme and glossing logic

Goal: If the imported data is a subset of an existing analysis then
 use the existing analysis, otherwise create a new analysis and add
 it to a matching Wordform.
# Conflicts:
#	Src/LexText/Interlinear/BIRDInterlinearImporter.cs
* Fix LT-22267: Slow response in interlinear combos

* Remove unused using
# cherry-picked from c531282
GenerateCssFromWsOptions() was only generating rules for one
writing system. Changed this to generate rules for all enabled
writing systems.
@jasonleenaylor jasonleenaylor marked this pull request as ready for review November 3, 2025 18:42
Copilot AI review requested due to automatic review settings November 3, 2025 18:42
@jasonleenaylor jasonleenaylor merged commit c0209df into master Nov 3, 2025
8 checks passed
@jasonleenaylor jasonleenaylor deleted the hotfix/9.2.11 branch November 3, 2025 18:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the BIRD format interlinear import logic to improve matching of existing wordforms and analyses, reducing duplicate creation. It also includes minor bug fixes and updates to build configuration.

  • Refactored CreateWordAnalysisStack into CreateWordformWithWfiAnalysis with improved matching logic that checks morphemes and glosses
  • Fixed GenerateCssFromWsOptions to accumulate all rules instead of returning on first match
  • Updated environment variable names in build tasks from Jenkins to GitHub Actions conventions

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
Src/xWorks/CssGenerator.cs Fixed early return bug by accumulating CSS rules for all writing systems
Src/MasterVersionInfo.txt Version bump from 9.2.10 to 9.2.11
Src/LexText/Interlinear/WordsSfmImportWizard.cs Added mainWs parameter to ImportWordsFrag call
Src/LexText/Interlinear/LinguaLinksImport.cs Updated method signatures to accept mainWs parameter and renamed method call
Src/LexText/Interlinear/ITextDllTests/ImportInterlinearAnalysesTests.cs Added comprehensive test coverage for morpheme matching and improved variable naming
Src/LexText/Interlinear/ITextDllTests/BIRDFormatImportTests.cs Removed ignored test case
Src/LexText/Interlinear/ChooseAnalysisHandler.cs Simplified gloss retrieval by removing guess services and reordered using statements
Src/LexText/Interlinear/BIRDInterlinearImporter.cs Major refactoring of analysis matching logic with new helper methods
Build/nuget-common/packages.config Added System.Buffers package dependency
Build/mkall.targets Added System.Buffers.dll to build configuration
Build/Src/FwBuildTasks/Substitute.cs Updated environment variable names for GitHub Actions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

LCModel.IText text;

IWfiWordform word = null;
IWfiWordform extandWordForm = null;
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'extand' to 'extant'.

Copilot uses AI. Check for mistakes.
var newWfiAnalysis = sl.GetInstance<IWfiAnalysisFactory>().Create();
word.AnalysesOC.Add(newWfiAnalysis);
segment.AnalysesRS.Add(word);
extandWordForm = sl.GetInstance<IWfiWordformFactory>().Create(wform);
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'extand' to 'extant'.

Copilot uses AI. Check for mistakes.
segment.AnalysesRS.Add(word);
extandWordForm = sl.GetInstance<IWfiWordformFactory>().Create(wform);
var extantAnalysis = sl.GetInstance<IWfiAnalysisFactory>().Create();
extandWordForm.AnalysesOC.Add(extantAnalysis);
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'extand' to 'extant'.

Copilot uses AI. Check for mistakes.
extandWordForm = sl.GetInstance<IWfiWordformFactory>().Create(wform);
var extantAnalysis = sl.GetInstance<IWfiAnalysisFactory>().Create();
extandWordForm.AnalysesOC.Add(extantAnalysis);
segment.AnalysesRS.Add(extandWordForm);
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'extand' to 'extant'.

Copilot uses AI. Check for mistakes.
Comment on lines 827 to 875
foreach (var wordItem in word.Items)
{
if (wordItem.Value == null)
continue;

var ws = GetWsEngine(wsFact, wordItem.lang);

switch (wordItem.type)
{
case "txt":
var wsAlt = GetWsEngine(wsFact, wordItem.lang);
if (wsAlt.Handle == wsMainVernWs.Handle)
wordForm = TsStringUtils.MakeString(wordItem.Value, ws.Handle);
expectedForms[ws.Handle] = wordItem.Value;

// Try to find a candidate wordform if we haven't found one yet
if (candidateForm == null)
{
candidateForm = cache.ServiceLocator
.GetInstance<IWfiWordformRepository>()
.GetMatchingWordform(ws.Handle, wordItem.Value);
}

break;

case "punct":
punctForm = TsStringUtils.MakeString(wordItem.Value, ws.Handle);
expectedForms[ws.Handle] = wordItem.Value;

if (candidateForm == null)
{
IPunctuationForm pf;
if (cache.ServiceLocator.GetInstance<IPunctuationFormRepository>()
.TryGetObject(punctForm, out pf))
{
candidateForm = pf;
}
}

break;

case "gls":
// Only consider human-approved glosses
if (wordItem.analysisStatusSpecified &&
wordItem.analysisStatus != analysisStatusTypes.humanApproved)
continue;
ITsString wffAlt = TsStringUtils.MakeString(wordItem.Value, wsAlt.Handle);
if (wffAlt.Length > 0)
wf.Form.set_String(wsAlt.Handle, wffAlt);

expectedGlosses[ws.Handle] = wordItem.Value;
break;
}
}
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Copilot uses AI. Check for mistakes.
itemDict[item.type] = new Tuple<string, string>(item.lang, item.Value);
}

if (itemDict.ContainsKey("txt")) // Morphemes
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inefficient use of 'ContainsKey' and indexer.
Inefficient use of 'ContainsKey' and indexer.

Copilot uses AI. Check for mistakes.
"New MorphBundles should not have been created.");

// Verify the imported analysis is the same object
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable importedPara may be null at this access because of this assignment.

Suggested change
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
Assert.NotNull(importedPara, "importedPara should not be null. Check that ParagraphsOS[0] is an IStTxtPara.");

Copilot uses AI. Check for mistakes.
"New MorphBundles should not have been created.");

// Verify the imported analysis is the same object
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable importedPara may be null at this access because of this assignment.

Suggested change
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
Assert.That(importedPara, Is.Not.Null, "ParagraphsOS[0] is not an IStTxtPara; importedPara is null.");

Copilot uses AI. Check for mistakes.
"Two new MorphBundles should have been created.");

// Verify the imported analysis and its contents
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable importedPara may be null at this access because of this assignment.

Suggested change
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
if (importedPara == null)
Assert.Fail("ParagraphsOS[0] is not an IStTxtPara as expected.");

Copilot uses AI. Check for mistakes.

// Verify the imported analysis and its contents
var importedPara = importedText.ContentsOA.ParagraphsOS[0] as IStTxtPara;
if(!(importedPara.SegmentsOS[0].AnalysesRS[0] is IWfiAnalysis importedAnalysis))
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable importedPara may be null at this access because of this assignment.

Suggested change
if(!(importedPara.SegmentsOS[0].AnalysesRS[0] is IWfiAnalysis importedAnalysis))
if (importedPara == null)
Assert.Fail("ParagraphsOS[0] is not an IStTxtPara");
else if (!(importedPara.SegmentsOS[0].AnalysesRS[0] is IWfiAnalysis importedAnalysis))

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants